feat(metrics): initialise MULTIPLEXED_METRIC_ROUTING_KEY for routing#19096
feat(metrics): initialise MULTIPLEXED_METRIC_ROUTING_KEY for routing#19096harshit078 wants to merge 22 commits intogetsentry:developfrom
Conversation
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
Hi @chargome, the PR is ready to review. Thanks ! |
chargome
left a comment
chargome
left a comment
There was a problem hiding this comment.
Nice, thanks for opening the PR! I left some comments. Also, we definitely want to test this behaviour somehow in a browser integration test.
| const container = Array.isArray(item) ? (item[1] as SerializedMetricContainer) : undefined; | ||
| const containerItems = container?.items; | ||
| if (containerItems) { | ||
| metric = containerItems[0]; |
There was a problem hiding this comment.
Wouldn't this mean we always just pull the first metric?
There was a problem hiding this comment.
yes it would mean we pull the first metric. What I'm thinking now after your comment is that I can add a logic which will check if all metrics and route to same or multiple destinations. Whats your opinion ?
| } | ||
|
|
||
| const transports = actualMatcher({ envelope, getEvent, getMetric }) | ||
| .map(result => { |
There was a problem hiding this comment.
Bug: A race condition occurs when sending metrics to multiple transports with different releases. The shared envelope is mutated concurrently, leading to unpredictable release values on metrics.
Severity: HIGH
Suggested Fix
To prevent the race condition, ensure that each transport operates on a unique copy of the envelope's items. Before applyReleaseToMetrics is called, deep-clone the envelope's items array (envelope[1]) so that mutations within one transport's send method do not affect others.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: packages/core/src/transports/multiplexed.ts#L146
Potential issue: A race condition exists when sending metrics to multiple transports
with different `release` versions. The `overrideDsn` function creates a new envelope
header for each transport but reuses the same reference to the envelope's items array.
When `makeOverrideReleaseTransport` calls `applyReleaseToMetrics`, it mutates this
shared array by replacing it with new metric objects containing the `release`. Because
multiple transports do this concurrently via `Promise.all`, the final `release` value on
the metrics is unpredictable, depending on which mutation finishes last. This leads to
data corruption where metrics can be assigned the wrong release version.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| } | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
Variable strippedCount incremented but never used
Low Severity
In stripRoutingAttributesFromMetrics, the variable strippedCount is declared and conditionally incremented via DEBUG_BUILD && strippedCount++ but is never used afterward. A reviewer mentioned adding a debug log here, suggesting this was intended for logging but was never completed. The variable serves no purpose without an accompanying log statement.
| export { createTransport } from './transports/base'; | ||
| export { makeOfflineTransport } from './transports/offline'; | ||
| export { makeMultiplexedTransport, MULTIPLEXED_TRANSPORT_EXTRA_KEY } from './transports/multiplexed'; | ||
| export { makeMultiplexedTransport } from './transports/multiplexed'; |
There was a problem hiding this comment.
Removed public export MULTIPLEXED_TRANSPORT_EXTRA_KEY is breaking change
Medium Severity
The constant MULTIPLEXED_TRANSPORT_EXTRA_KEY was previously exported from @sentry/core and @sentry/browser but has been removed from the public exports. Existing consumers who import this constant will experience a breaking change. Per the review rules, removal of publicly exported constants requires proper deprecation notices.
Additional Locations (1)
2 participants
Before submitting a pull request, please take a look at our
Contributing guidelines and verify:
yarn lint) & (yarn test).Closes #18913
What changed
metricsMetricRoutingInfowith dsn and release as its fieldMetricOptionsand enabledcaptureMetricto inject routing info into attributesInternalCaptureMetricOptions_stripRoutingAttributesfunction which removes routing info before sending to sentry